[Kotlin] Prevent repeated checkout presentation crashes - #600
[Kotlin] Prevent repeated checkout presentation crashes#600kieran-osgood-shopify wants to merge 3 commits into
Conversation
kieran-osgood-shopify
changed the title
kieran-osgood-shopify
changed the title
| private val activity: ComponentActivity, | ||
| val handle: CheckoutHandle, | ||
| ) { | ||
| fun isShowingFor(context: ComponentActivity): Boolean = activity === context && sheet.isShowing |
There was a problem hiding this comment.
Wondering here if the activity === context `check is needed?
Could we still stack if presenting from a separate activity? Is that problematic?
fun isShowing(): Boolean = sheet.isShowingor directly without the function
val alreadyPresented = livePresentation?.takeIf { it.sheet.isShowing }Maybe with docs to say only one checkout can be presented per SDK process
There was a problem hiding this comment.
Good Q - the check gets us closest to the swift implementation, where they drop additional ones on the viewcontroller, it doesn't guard against other viewcontrollers presenting, or the presented via presenting another modal view
I was hoping to avoid a "global" lock situation - what do you think, are you leaning towards a global 1 at a time?
There was a problem hiding this comment.
Hey, yeah it sounds fair to aim for parity with swift
Is there currently a hole with a single livePresentation activity check, where presenting on B overwrites A’s tracked presentation, so a subsequent present on A can still stack a second A sheet.
// maybe we should keep multiple slots
private val livePresentations = mutableMapOf<ComponentActivity, LivePresentation>()
// instead of
private var livePresentation: LivePresentation? = nullSo we can track active presentations per activity (and return the existing handle only for that activity)?
d84281e to
99e2e88
Compare
kieran-osgood-shopify
changed the title
### TLDR `present` built a new `CheckoutBottomSheet` on every call, so N taps on a checkout button stacked N dialog windows. A device capture showed 73 `present` calls producing 73 sheets, two of them only 153 ms apart. ### What `ShopifyCheckoutKit` now tracks the live presentation. A second `present` for the same activity logs a warning and returns the existing `CheckoutHandle` instead of building another sheet. A stored sheet that is no longer showing counts as stale, so the next `present` proceeds normally. `CheckoutBottomSheet` gains an internal `onDismissFinalized` callback, invoked in `finishDismiss` above the `isShowing` guard, so every dismissal path clears the slot: buyer dismissal, gesture dismissal, programmatic `dismiss`, lifecycle teardown, and `closeCheckoutWithError`. That callback also removes the per-presentation `DefaultLifecycleObserver`. Before this change the observer was removed only when `start()` failed, so each successful presentation leaked one observer holding a dead sheet. No public API change. Both new members are private, or internal on an internal class, so `lib/api/lib.api` does not move. ### How to test Add an item to the cart in the Kotlin sample, then tap Checkout many times quickly. | | Before | After | | --- | --- | --- | | Sheets shown per burst | 5 | 1 | | Leaked sheet windows after closing | 4 | 0 | Leaked sheet windows are `androiddemo` entries in `dumpsys window windows` with `ty=APPLICATION gr=BOTTOM`. Before this change two surfaceless sheet windows survived every close, and one of them still held `mCurrentFocus`. The app then drew nothing, without crashing. `ShopifyCheckoutKitTest.kt` covers the refusal, the returned handle identity, presenting again after a dismissal, and the observer count returning to its starting value.
99e2e88 to
37bce2f
Compare
Package Size
Android file breakdown
Measured from the PR base SHA and PR head SHA. The file breakdown shows uncompressed sizes within each package artifact, so individual files do not sum to the compressed artifact total. This comment reports package artifact sizes only; it is not a final app binary-size report. |
Install this buildOpen Tophat, select your target device, then click Install. Links open on the Mac running Tophat.
Checkout Kit E2E results
|
Assisted-By: devx/ae9b9301-933e-49a4-922e-fd6923880697
2 participants
What changes are you making?
Repeated checkout presentation can fail in two related ways:
presentcalls while a checkout is visible can stack bottom-sheet windows.SIGSEGVfrom a null pointer dereference inlibmonochrome_64.so.This change:
CheckoutHandlewhenpresentis called again for the same activity.There is no public API change.
Original stacked-sheet recording (uploaded via Graphite)
How to test
The regression test preloads checkout, presents it, dismisses it, and asserts that the dismissed WebView is destroyed and the next presentation uses a fresh WebView.
Manual verification re-presented checkout 243 ms after gesture dismissal on a Pixel 9 Pro Android 16 emulator. The same app process remained alive and the second checkout rendered.
Validation
dev android testdev android lintdev android api checkdev android formatBefore you merge
Important